Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: app hangs on starlette when sufficiently many cookies are set #751

Merged
merged 2 commits into from
Aug 26, 2024

Conversation

iisakkirotko
Copy link
Collaborator

Couldn't reproduce locally. Giving ci a shot.

Copy link

render bot commented Aug 26, 2024

Copy link
Collaborator Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @iisakkirotko and the rest of your teammates on Graphite Graphite

@maartenbreddels
Copy link
Contributor

image

From https://github.com/widgetti/solara/actions/runs/10484645065/job/29169659662?pr=743

I added branch protection so we can auto merge, but that didn't let us do the auto commit. I had to disable it, so the updates ran on master. Maybe that was the same issue?

@EwoutH could you try to open a new trivial PR so we can test this?

In version 13.0 of `websockets`, the environmental variables to configure websockets were renamed (see [here](https://websockets.readthedocs.io/en/stable/project/changelog.html#id2)), which caused apps to hang when running on starlette with more than 8kb of cookies set.
@iisakkirotko iisakkirotko force-pushed the 08-26-ci_automatic_testing_suite_failing branch from 36a9641 to 165841b Compare August 26, 2024 11:35
@iisakkirotko iisakkirotko changed the title ci: automatic testing suite failing fix: app hangs on starlette when sufficiently many cookies are set Aug 26, 2024
@EwoutH
Copy link
Contributor

EwoutH commented Aug 26, 2024

@EwoutH could you try to open a new trivial PR so we can test this?

Of course, but I now see CI passing, so is it still needed?

Edit: If so, what do I need to do exactly?

@iisakkirotko iisakkirotko marked this pull request as ready for review August 26, 2024 12:51
@iisakkirotko
Copy link
Collaborator Author

Hey @EwoutH! You're right, this issue turned out to be unrelated. I'll ping you to test stuff when I get around to working on #750 :)

@maartenbreddels maartenbreddels merged commit 4647131 into master Aug 26, 2024
2 checks passed
@maartenbreddels
Copy link
Contributor

Thanks @iisakkirotko !

Sorry, I misunderstoof @EwoutH , it seems 3 (!) related issues were confused in my mind

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants